-
Notifications
You must be signed in to change notification settings - Fork 11.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Chore: Upgrade Go version to 1.19.1 (backport) #55733
Conversation
Backend code coverage report for PR #55733
|
Frontend code coverage report for PR #55733 |
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/34899 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM I have spotted some minor issues with the swagger generation.
pkg/api/api.go
Outdated
// | ||
// type: basic | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be valid yaml otherwise the make swagger-api-spec
fails with:
yaml: unmarshal errors:
line 4: mapping key "type" already defined at line 2
// | |
// type: basic | |
// | |
// type: basic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go fmt
won't allow that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can fix it by making all lines in the yaml indented, but then I get
SWAGGER_GENERATE_EXTENSION=false /Users/sakjur/go/bin/swagger-v0.30.2 generate spec -m -w pkg/server -o public/api-spec.json \
-x "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" \
-x "github.com/prometheus/alertmanager" \
-i pkg/api/swagger_tags.json
yaml: line 2: mapping values are not allowed in this context
make: *** [--swagger-api-spec] Error 1
We can't really not upgrade, fwiw, so unless we'll find a good solution to making Swagger work for v9.1.x, I'd suggest breaking Swagger for the v9.1.x branch (it should be finalized for that version around the time of v9.1.0, so it wouldn't be the weirdest of things to do)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid breaking swagger is not an option unless build-go target no more depends on that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is how it's in main and seems to be OK both with gofmt
and swagger
https://github.com/grafana/grafana/blob/main/pkg/api/api.go#L1-L30
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, main has a tab before basic
as well, it's not just this line that's changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this now, but I'm still getting the yaml: line 2: mapping values are not allowed in this context
error and I don't get how to track it down?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have pushed some changes that work for me locally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏 Thank you!
@@ -126,7 +126,7 @@ RUN apt-get update && \ | |||
gcc \ | |||
g++ \ | |||
git \ | |||
jq \ | |||
jq \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, it's me aligning this, but missing that the first whitespace on the jq
was a tab and not four spaces
Co-authored-by: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com>
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/34932 |
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/34940 |
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/34948 |
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/34982 |
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/35069 |
What this PR does / why we need it:
Upgrades the version of Go used to build Grafana v9.1.x to 1.19.1.